-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix display of values in settings panel #2263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b2d1c6e to
bb7ab54
Compare
matthiask
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Can you show an example of how it looks now?
debug_toolbar/panels/settings.py
Outdated
| { | ||
| "settings": { | ||
| key: force_str(value) | ||
| key: pformat(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that force_str can just be removed? I think we added that to not crash when value contains binary data or something. Maybe pformat handles that just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. It should return a string for most well-behaved objects but I can wrap it so it tolerates exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has just been added here: https://github.com/django-commons/django-debug-toolbar/pull/2217/files
The bytes.fromhex value would probaly be a good test for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pprint.pformat would format that bytes value as:
(b'\xa3\xf2\xb8\xc1N\x97-Z\x8f\xb3\xc7)\x1ad\xe0\x85\x9cG+\xf6=\x18\xa0\x94'
b'^s\xb2\xc8O\x91z\xe2')
instead of "Django Debug Toolbar was unable to parse value."
| ``debug_toolbar.store.DatabaseStore`` with ``SKIP_TOOLBAR_QUERIES``. | ||
| * Fixed font family for code blocks and stack traces in the toolbar. | ||
| * Added test to confirm Django's ``TestCase.assertNumQueries`` works. | ||
| * Fixed display of values in settings panel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe more specific? Not sure if that's an improvement, just a suggestion.
| * Fixed display of values in settings panel. | |
| * Fixed double quoting of values in settings panel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work better?
Fixed string representation of values in settings panel
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
|
@matthiask the attached screenshot shows how it looks with this change. I added one with the current behavior just below. |
I'm sorry if I'm being dense but I only see one screenshot? |
bb7ab54 to
105b605
Compare
|
How about now? |
Description
There was some weird double quoting going on in the settings panel values column. This fixes the formatting so it's more consistent and clearly distinguishes string from non-string values. Added some tests for the new behavior.
Before
After
Checklist:
docs/changes.rst.